Skip to content

fix: Use proper path APIs in Folder.CopyTo() instead of string replacement#1462

Merged
thomhurst merged 9 commits into
mainfrom
fix/issue-1453-folder-copyto
Dec 30, 2025
Merged

fix: Use proper path APIs in Folder.CopyTo() instead of string replacement#1462
thomhurst merged 9 commits into
mainfrom
fix/issue-1453-folder-copyto

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Replaced brittle string replacement (dirPath.Replace(this, targetPath)) with proper path APIs (Path.GetRelativePath() and Path.Combine())
  • Fixes correctness and security issues when source path contains the same substring multiple times
  • The previous implementation could corrupt paths in edge cases

Changes

Before:

Directory.CreateDirectory(dirPath.Replace(this, targetPath));
System.IO.File.Copy(newPath, newPath.Replace(this, targetPath), true);

After:

var relativePath = System.IO.Path.GetRelativePath(this, dirPath);
var newPath = System.IO.Path.Combine(targetPath, relativePath);
Directory.CreateDirectory(newPath);

Fixes #1453

Test plan

  • Build succeeds
  • Existing tests pass
  • The fix correctly handles edge cases where source path contains repeated substrings

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown
Contributor

Summary

Fixes path corruption bug in Folder.CopyTo() by replacing string replacement with proper path APIs.

Critical Issues

None found ✅

Suggestions

None - the fix correctly addresses the issue described in #1453 using the appropriate .NET path APIs.

Verdict

APPROVE - No critical issues


Review Notes:

  • The original implementation using string.Replace() was indeed brittle and could corrupt paths when the source path substring appeared multiple times in a directory/file path
  • Using Path.GetRelativePath() + Path.Combine() is the correct approach for this operation
  • Variable rename from newPath to filePath in the file enumeration loop improves clarity
  • The fix is minimal, focused, and follows .NET best practices

@thomhurst thomhurst enabled auto-merge (squash) December 30, 2025 02:44
@thomhurst thomhurst merged commit a39cee9 into main Dec 30, 2025
10 of 12 checks passed
@thomhurst thomhurst deleted the fix/issue-1453-folder-copyto branch December 30, 2025 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CRITICAL: Folder.CopyTo() uses brittle string replacement for path manipulation

1 participant